Fix #225: allow disabling the RDP idle timeout (0 = never disconnect)#272
Open
eran132 wants to merge 1 commit into
Open
Fix #225: allow disabling the RDP idle timeout (0 = never disconnect)#272eran132 wants to merge 1 commit into
eran132 wants to merge 1 commit into
Conversation
The RDP idle timeout was clamped to 10-240, and the config layer's getter forced any value below 10 up to 10, so entering 0 or -1 silently became 10. Users running long-lived sessions had no way to stop idle disconnects. Treat 0 as "disabled": RdpTimeOutOptions.IdleTimeout and FavoriteConfigurationElement.IdleTimeout now allow 0 to round-trip (it maps to MsRdpClient AdvancedSettings.MinutesToIdleTimeout = 0, i.e. no timeout). Positive values are still clamped to the MsRdpClient range 10-240. Also aligned the config getter's upper bound to 240 (was an inconsistent 600). The RDP Extended settings label now reads "Idle Timeout (0=off)". Adds unit tests for both clamping layers (0/-1 -> 0, <10 -> 10, >240 -> 240, in-range preserved, default 240). Fixes Terminals-Origin#225 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support (and coverage) for disabling RDP idle timeout by allowing an IdleTimeout value of 0, while retaining clamping behavior for other values.
Changes:
- Added a dedicated unit test suite for
RdpTimeOutOptionsidle-timeout normalization/clamping. - Updated
RdpTimeOutOptions.IdleTimeoutto preserve0as “disabled” and normalize negatives to0. - Updated
FavoriteConfigurationElement.IdleTimeoutclamping behavior and added corresponding tests.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Tests/Tests.csproj | Includes the new RdpTimeOutOptionsTests file in the test project build. |
| Source/Tests/Connections/RdpTimeOutOptionsTests.cs | Adds coverage for IdleTimeout semantics (0 disable, negative normalize, min/max clamping). |
| Source/Tests/Configuration/FavoriteConfigurationElementTests.cs | Adds tests validating config element IdleTimeout round-trip behavior, including the new “0 disables” rule. |
| Source/Terminals.Plugins.Rdp/RdpTimeOutOptions.cs | Implements “0 disables idle timeout” and updates documentation accordingly. |
| Source/Terminals.Common/Configuration/FavoriteConfigurationElement.cs | Adjusts config element clamping to allow 0 and clamp max to 240. |
Files not reviewed (1)
- Source/Terminals.Plugins.Rdp/RdpExtendedSettingsControl.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
295
to
308
| public Int32 IdleTimeout | ||
| { | ||
| get | ||
| { | ||
| Int32 val = (Int32)this["idleTimeout"]; | ||
| if (val > 600) | ||
| val = 600; | ||
| if (val > 240) | ||
| val = 240; | ||
|
|
||
| if (val < 10) | ||
| val = 10; | ||
| // 0 means the idle timeout is disabled (never disconnect), so keep it as is. | ||
| if (val < 0) | ||
| val = 0; | ||
|
|
||
| return val; | ||
| } |
Comment on lines
+113
to
+119
| [TestMethod] | ||
| public void IdleTimeoutZero_SetGet_StaysZeroToDisableTimeout() | ||
| { | ||
| FavoriteConfigurationElement favorite = this.CreateFavorite(); | ||
| favorite.IdleTimeout = 0; | ||
| Assert.AreEqual(0, favorite.IdleTimeout, "0 means the idle timeout is disabled and must survive a set/get round-trip (issue #225)."); | ||
| } |
Comment on lines
62
to
72
| public Int32 IdleTimeout | ||
| { | ||
| get | ||
| { | ||
| return CorrectValueToInterval(10, 240, idleTimeout); | ||
| return idleTimeout <= 0 ? 0 : CorrectValueToInterval(10, 240, idleTimeout); | ||
| } | ||
| set | ||
| { | ||
| idleTimeout = CorrectValueToInterval(10, 240, value); | ||
| idleTimeout = value <= 0 ? 0 : CorrectValueToInterval(10, 240, value); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #225. There was no way to stop RDP sessions from being disconnected for being idle — the idle timeout was clamped to 10–240, and entering
0or-1was silently forced back up to10.Root cause was a clamp in two places:
RdpTimeOutOptions.IdleTimeoutclamped to[10, 240]on both get and set.FavoriteConfigurationElement.IdleTimeoutsetter already allowed0, but its getter forced any value< 10up to10— so even a stored0read back as10.Change
Treat
0as "disabled" in both layers, so it round-trips intact. It maps directly to the MsRdpClient control:MinutesToIdleTimeout = 0means no idle timeout (documented MsRdpClient range is 0–240). Positive values are still clamped to[10, 240], so existing behavior is unchanged for them. I also aligned the config getter's upper bound to240(it was an inconsistent600). The RDP Extended settings label now readsIdle Timeout (0=off)so the option is discoverable.Tests
Added unit tests for both clamping layers (
RdpTimeOutOptionsTests, plus cases inFavoriteConfigurationElementTests):0and-1→0, a positive value< 10→10,> 240→240, in-range preserved, default240. Verified they fail before the change (0/-1 became 10) and pass after. Full suite shows no new failures (the only pre-existing failures locally are the SQL/LocalDB persistence tests, which need a database).🤖 Generated with Claude Code